Skip to content

feat(ai): support project-defined skills (skills/*.md) for AI agents - #9851

Merged
nishantmonu51 merged 19 commits into
mainfrom
nishant/ai-skills
Sep 15, 2026
Merged

nishantmonu51 merged 19 commits into
mainfrom
nishant/ai-skills

Conversation

@nishantmonu51

@nishantmonu51 nishantmonu51 commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator

Adds skills: instruction files that teach Rill's AI agents project-specific practices, such as root-cause-analysis playbooks and business glossaries. This is the inbound counterpart of the SKILL.md format runtime/ai/instructions already emits for Claude/Cursor.

  • Skills follow the Agent Skills format: a directory with a SKILL.md file at skills/<name>/SKILL.md (also loaded from .agents/skills/ for cross-client compatibility), with front matter name (must match the directory) and description (drives selection). Rill adds extension fields metrics_views (relevance scoping), agents, and always_apply, which other clients ignore.
  • Skills are parsed in runtime/parser into a new Skill resource kind and inserted into the catalog by the project parser, so the AI session reads them from the catalog rather than the repo. Invalid skill files surface as parse errors on the file, and a skill scoped to a nonexistent metrics view gets a reconcile error.
  • Progressive disclosure: the analyst agent's prompt gets an index of relevant skills (scope-filtered by the dashboard's metrics views) and calls the new load_skill tool on demand; always_apply bodies are inlined after ai_instructions (32kb cap).
  • New list_skills/load_skill tools are gated on UseAI only, so cloud viewers without ReadRepo can use them; both are exposed on the MCP server automatically, and the server instructions tell external clients to discover and load skills.
  • Chat UI renders load_skill in the thinking trace ("Loading skill..." / "Loaded skill" labels ship server-side via tool meta); Add → More gains an "AI Skill" entry with a starter template, and skill files get a dedicated icon in the file explorer.
  • Docs: new Skills section in ai-configuration.md (format, scoping, always_apply vs ai_instructions, and a warning that skill content is visible to all AI-enabled members) plus MCP guide updates.
  • Tests: parser parsing/validation/reparse coverage, tool behavior and viewer-claims access, MCP exposure, and a TestAnalystSkills eval asserting load_skill is called and the playbook is followed.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L

Skills are markdown files at skills/<name>.md or skills/<name>/SKILL.md
with YAML front matter (description required; optional name, metrics_views,
agents, always_apply) that teach Rill's AI agents project-specific practices
such as analysis playbooks and business glossaries.

Runtime:
- New session-scoped loader in runtime/ai reads skills from the repo;
  malformed files are reported as issues and logged, never failing the session.
- New list_skills and load_skill tools gated on UseAI only,
  so cloud viewers without repo access can use them;
  they are exposed on the MCP server automatically for external clients.
- The analyst agent's prompt gains an index of relevant skills
  (scope-filtered by the dashboard's metrics views) and inlines always_apply
  skill bodies after ai_instructions, capped at 32kb.
- The MCP server instructions tell clients to discover and load skills.
- instructions.ParseFrontMatter is exported and shared with the embedded instructions.

Frontend:
- load_skill/list_skills render in the chat thinking trace with a skill icon.
- Add > More gains an "AI Skill" entry that creates skills/my_skill.md from
  a starter template; skill files get a dedicated icon in the file explorer.

Claude-Session: https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L
…ctions for external clients

Piggybacks always_apply skill bodies on the ai_instructions field that
list_metrics_views returns to external MCP clients (restored on main),
so clients receive glossary-style skills on their first call without
relying on the server instructions to load them.
Internal rill sessions are excluded since their prompts already inline these skills.

Claude-Session: https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L
The soft "check whether any skill matches" wording let the model skip
load_skill on questions a skill clearly covered; instruct it to load a
matching skill before running any queries.

Claude-Session: https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L
The analyst prompt already injects nothing when a project defines no skills;
apply the same to the per-session MCP server, which advertised a static
skills section regardless. The exported MCPInstructions keeps the full text
for the unified admin MCP server, which cannot tailor per project.

Claude-Session: https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L
- `name:` (optional) overrides the name derived from the file path
- `metrics_views:` (optional) list of metrics view names; the skill is only offered when the analysis involves one of them
- `agents:` (optional) list of agents the skill applies to, `analyst` and/or `developer`; defaults to `[analyst]`
- `always_apply:` (optional) if `true`, the skill's full body is always injected into the agent's context instead of being loaded on demand; use for short, broadly applicable guidance such as glossaries

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like its slightly different from the agent skills standard. Consider making it fully compatible with the standard: https://agentskills.io/specification

Also, would it make sense to load from the generic .agents/skills instead of skills? For compatibility with other chat clients like Claude.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0db9c94: skills now follow the Agent Skills spec (<name>/SKILL.md, name must match the directory, standard optional fields accepted; Rill's metrics_views/agents/always_apply are extension fields). Also loaded from .agents/skills/.

Comment thread runtime/ai/skills.go Outdated
Comment on lines +85 to +95
func loadSkills(ctx context.Context, rt *runtime.Runtime, instanceID string) ([]*Skill, []SkillIssue, error) {
repo, release, err := rt.Repo(ctx, instanceID)
if err != nil {
return nil, nil, fmt.Errorf("failed to open repo: %w", err)
}
defer release()

entries, err := repo.ListGlob(ctx, skillsGlob, true)
if err != nil {
return nil, nil, fmt.Errorf("failed to list skill files: %w", err)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best to avoid reading files directly from server code. The repo may be unavailable (e.g. Github outage, disconnected repo, slow clone due to large files, etc.), and we don't want it to cause APIs to time out or error. The only code that normally reads from the repo directly is the reconcilers, which then write state into the catalog; then API code normally only hits the catalog to stay reliable.

So a better option may be to parse skills in runtime/parser, and write them into the catalog in e.g. runtime/reconcilers/project_parser.go, and then read from the catalog here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0db9c94: skills are parsed in runtime/parser into a new Skill resource, written to the catalog by the project parser reconciler, and the AI code reads only from the catalog.

Comment thread runtime/ai/skills.go Outdated
}

var fm skillFrontMatter
body, err := instructions.ParseFrontMatter([]byte(content), &fm)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instructions directly is specifically made for instructions in runtime/ai/instructions/data, maybe consider having separate/dedicated code for the user-facing skills parsing so the two use cases don't get too coupled.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0db9c94: skill parsing lives in runtime/parser/parse_skill.go with no dependency on runtime/ai/instructions; the ParseFrontMatter export is reverted.

… Skills format

Addresses PR review feedback:
- Skills are now parsed in runtime/parser into a new Skill resource kind, inserted into the catalog by the project parser, and read from the catalog by the AI session, so API code no longer reads the repo directly.
- The file format follows the Agent Skills standard (https://agentskills.io): a skill is a directory with a SKILL.md file, the name must match the directory, and the standard optional fields are accepted. Rill's metrics_views/agents/always_apply remain as extension fields that other clients ignore. Skills are also loaded from .agents/skills/ for cross-client compatibility.
- Skill parsing no longer depends on the runtime/ai/instructions package; its ParseFrontMatter export is reverted.
- Invalid skill files now surface as parse errors on the file, and a skill scoped to a nonexistent metrics view gets a reconcile error.

Claude-Session: https://claude.ai/code/session_017udazBgdXmdTXMTq7sTh2L
- Fix `TestAnalystSkills` to use the `skills/<name>/SKILL.md` layout, which the flat
  `skills/<name>.md` paths stopped matching after the Agent Skills refactor.
- Forward `list_skills` and `load_skill` on the Cloud unified MCP server, which
  advertised the skills instructions without exposing the tools they reference.
- Generate unique skill directory names with hyphens instead of post-processing
  `getName`'s `_N` suffix, which could produce an existing name and silently fail
  the create.
- List skills with a dedicated `**/SKILL.md` glob so unrelated markdown files
  don't count against `drivers.RepoListLimit` and fail the whole parse.
- Wire skills into the developer agent, so `agents: [developer]` is no longer
  inert, and share the prompt-building logic with the analyst agent.
- Filter skills by agent in `list_metrics_views`, so developer-only skills don't
  leak into the `ai_instructions` served to external clients.
- Require `UseAI` to access a skill resource, and note in the docs that this
  includes anonymous visitors on public projects.
- Report empty skill front matter as a missing `description` instead of an
  unclosed delimiter.
- Resolve the skills instructions during the MCP initialization handshake instead
  of on every stateless request.

Claude-Session: https://claude.ai/code/session_01WTWACZUSP88ypUfVb8FbJr
@nishantmonu51 nishantmonu51 added Type:Feature New feature request Size:L Large change: 500-1,999 lines labels Sep 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Invalid and incorrectly scoped skills can still reach agents, and several parsing and localization issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds project-defined AI skills across parsing, reconciliation, agent prompts, MCP tooling, UI, and documentation.

Changes:

  • Parses and validates SKILL.md resources with agent and metrics-view scoping.
  • Adds skill discovery/loading to internal agents and MCP clients.
  • Adds creation, icons, generated APIs, tests, and documentation.
File summaries
File Description
web-common/src/runtime-client/gen/index.schemas.ts Adds generated skill schemas.
web-common/src/proto/gen/rill/runtime/v1/resources_pb.ts Adds generated skill protobuf classes.
web-common/src/features/entity-management/resource-selectors.ts Registers the skill resource kind.
web-common/src/features/entity-management/resource-icon-mapping.ts Adds skill labels and icons.
web-common/src/features/entity-management/add/new-files.ts Defines the starter skill template.
web-common/src/features/entity-management/add/AddAssetButton.svelte Adds the AI Skill creation action.
web-common/src/features/chat/core/types.ts Registers skill tool names.
web-common/src/features/chat/core/messages/tools/tool-icons.ts Maps skill tools to icons.
runtime/server/mcp_test.go Tests conditional MCP skill instructions.
runtime/security.go Grants skill access to AI users.
runtime/resources.go Registers runtime skill conversions.
runtime/reconcilers/skill.go Validates skill metrics-view references.
runtime/reconcilers/project_parser.go Reconciles parsed skills into the catalog.
runtime/parser/parser.go Integrates skill files into parsing and reparsing.
runtime/parser/parser_test.go Extends resource assertions for skills.
runtime/parser/parse_skill.go Implements skill parsing and validation.
runtime/parser/parse_skill_test.go Tests skill parsing and reparsing.
runtime/ai/skills.go Loads, filters, and formats skills.
runtime/ai/skills_test.go Tests skill tools, validation, and access.
runtime/ai/skills_internal_test.go Tests filtering and MCP instructions.
runtime/ai/skill_load.go Implements load_skill.
runtime/ai/skill_list.go Implements list_skills.
runtime/ai/project_status.go Includes skills in project status.
runtime/ai/metrics_view_list.go Adds always-apply skills for MCP clients.
runtime/ai/mcp.go Advertises skill tools and instructions.
runtime/ai/instructions/instructions.go Generalizes front-matter decoding.
runtime/ai/instructions/instructions_test.go Updates front-matter tests.
runtime/ai/instructions/data/development.md Documents skill files for the developer agent.
runtime/ai/developer_agent.go Adds developer skill prompting and loading.
runtime/ai/analyst_agent.go Adds scoped analyst skills.
runtime/ai/analyst_agent_test.go Adds an analyst skill evaluation.
runtime/ai/ai.go Registers tools and session skill cache.
proto/rill/runtime/v1/resources.proto Defines the skill resource schema.
proto/gen/rill/runtime/v1/runtime.swagger.yaml Adds generated OpenAPI definitions.
proto/gen/rill/runtime/v1/resources.pb.validate.go Adds generated validation types.
docs/docs/guide/ai/mcp.md Documents MCP skill tools.
docs/docs/developers/build/ai-configuration.md Documents skill configuration and security.
admin/server/mcp.go Forwards skill tools through admin MCP.
Review details

Files not reviewed (1)

  • proto/gen/rill/runtime/v1/resources.pb.validate.go: Generated file

Suppressed comments (1)

runtime/parser/parse_skill.go:144

  • This search accepts any line beginning with --- (for example ---- or --- trailing) as the closing delimiter, so malformed Agent Skills front matter is silently accepted and part of that line moves into the body. Match an exact delimiter line so invalid skill files surface the promised parse error.
	endIdx := strings.Index(rest, "\n---")
	if endIdx == -1 {
		return "", errors.New(`unclosed front matter: missing closing "---" line`)
	}
	frontMatter := rest[:endIdx]
	body := strings.TrimSpace(rest[endIdx+len("\n---"):])
  • Files reviewed: 37/39 changed files
  • Comments generated: 7
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread runtime/ai/analyst_agent.go Outdated
Comment thread runtime/ai/skills.go
Comment thread runtime/ai/skills.go Outdated
Comment thread runtime/parser/parse_skill.go
Comment thread web-common/src/features/entity-management/add/AddAssetButton.svelte
Comment thread web-common/src/features/entity-management/resource-icon-mapping.ts
Comment thread web-common/src/features/entity-management/resource-selectors.ts
- Resolve explore/canvas metrics views on every analyst invocation so skill scoping works on follow-ups
- Skip skills with reconcile errors when loading them into the session
- Match metrics view scopes case-insensitively
- Reject whitespace-only skill descriptions and require an exact "---" front matter delimiter

Claude-Session: https://claude.ai/code/session_014SrRA996FvGTcB891M5sWS

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Generic YAML skill types can panic parsing, and prompt-size checks do not fully enforce their cap.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • proto/gen/rill/runtime/v1/resources.pb.validate.go: Generated file
  • Files reviewed: 37/39 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread runtime/parser/parser.go Outdated
Comment thread runtime/ai/metrics_view_list.go Outdated
Comment thread runtime/ai/skills.go Outdated
- Keep `skill` out of the generic YAML kind parser so `type: skill` yields a parse error instead of a panic
- Apply the always-apply size cap to the fully rendered section, including headings and separators

Claude-Session: https://claude.ai/code/session_014SrRA996FvGTcB891M5sWS

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Skill creation can collide across supported roots, Unicode validation is incorrect, and the generated skill index is unbounded.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • proto/gen/rill/runtime/v1/resources.pb.validate.go: Generated file

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

runtime/parser/parse_skill.go:87

  • len counts UTF-8 bytes, while the validation error and Agent Skills limit are expressed in characters. A valid description containing 1024 non-ASCII characters can therefore be rejected. Count Unicode code points instead.
    docs/docs/developers/build/ai-configuration.md:121
  • The Agent Skills format requires name; describing it as optional encourages files that other Agent Skills clients reject, which contradicts the cross-client compatibility claim below. Rill may keep lenient ingestion, but the guide should tell authors to include the required field.
    runtime/ai/instructions/data/development.md:199
  • This embedded developer guidance also labels the Agent Skills name field optional, so the developer agent may author a Rill-only file that other compatible clients reject. Instruct it to emit the required name field even if Rill's parser remains lenient.
  • Files reviewed: 37/39 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread runtime/ai/skills.go Outdated
Comment thread web-common/src/features/entity-management/add/AddAssetButton.svelte
Comment thread docs/docs/developers/build/ai-configuration.md Outdated
Comment thread web-common/src/features/entity-management/add/new-files.ts Outdated
- Cap the on-demand skill index and point the agent to list_skills for the rest
- Count the description limit in characters rather than bytes
- Avoid skill name collisions with .agents/skills when creating a skill in the UI
- Document that name is required by the Agent Skills format and that always_apply falls back to on-demand loading past the budget

Claude-Session: https://claude.ai/code/session_014SrRA996FvGTcB891M5sWS
@nishantmonu51
nishantmonu51 requested a balanced review from Copilot September 11, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nishantmonu51
nishantmonu51 requested a balanced review from Copilot September 13, 2026 17:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate findings affect Skill discovery, access scoping, parser limits, and frontend error reporting.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • proto/gen/rill/runtime/v1/resources.pb.validate.go: Generated file

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

web-common/src/features/entity-management/resource-selectors.ts:36

  • Adding Skill as a catalog resource does not update the file-artifact initialization whitelist in file-artifacts.ts. On a reload, existing SKILL.md files therefore never receive a resourceName, so getAllErrors cannot fetch a skill's reconcile error (for example, a missing scoped metrics view) and the file explorer shows no resource error. Include ResourceKind.Skill in that initialization path.

runtime/ai/mcp.go:128

  • A caller without UseAI can still create this MCP session when it has access to other read-only tools, and the skill tools are then omitted by their CheckAccess gates. This branch nevertheless advertises the Skills section whenever the project has a skill, so those clients are instructed to call tools they cannot use; gate this condition on the session's UseAI permission.
			if skillsErr != nil || len(skills) > 0 {
				init.Instructions = MCPInstructions

runtime/ai/metrics_view_list.go:115

  • ListMetricsViews.CheckAccess only requires ReadObjects, but this new branch appends sk.Body to ai_instructions. A caller without UseAI can therefore retrieve project skill contents through list_metrics_views, bypassing the new skill-tool gate. Guard this enrichment with session.Claims().Can(runtime.UseAI) while retaining the existing metrics-view access.
		skills, err := session.Skills(ctx)
		if err != nil {
			session.logger.Warn("failed to load project skills", zap.Error(err))
		}
		for _, sk := range filterSkills(skills, parser.SkillAgentAnalyst, nil) {

runtime/ai/metrics_view_list.go:118

  • This handler has no selected metrics view, and filterSkills(..., nil) intentionally treats an empty context as matching every scoped skill. As a result, an always_apply skill scoped to metrics_views: [orders] is injected into the global ai_instructions returned for unrelated external MCP analyses. Skip scoped skills here and leave them for list_skills/load_skill once a matching view is known.
		for _, sk := range filterSkills(skills, parser.SkillAgentAnalyst, nil) {
			if !sk.AlwaysApply {
				continue
			}

runtime/ai/skills.go:131

  • When the index exceeds its cap, this tells the agent to call list_skills, but both analyst_agent.go and developer_agent.go only add LoadSkillName to their Complete tool allowlists. The LLM therefore cannot invoke the advertised discovery tool, leaving omitted skills (including oversized always_apply fallbacks) undiscoverable unless it already guesses their names. Add ListSkillsName to those agent tool lists or change this fallback.
		fmt.Fprintf(&indexBuf, "- (%d more skills not listed here; call %s to see them)\n", omitted, ListSkillsName)

runtime/parser/parser.go:387

  • **/SKILL.md scans every file with that basename, including .claude/skills, nested skill references, and unrelated directories, even though skillNameForPath only accepts the two direct roots. Those entries still count toward RepoListLimit and maxFiles, so a repository with many non-Rill SKILL.md files can fail reload before valid resources are parsed. Use separate direct-root globs (or filter before applying the limits).
	skillFiles, err := p.Repo.ListGlob(ctx, "**/SKILL.md", true)
  • Files reviewed: 37/39 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +187 to +189
if len(skills) > 0 {
tools = append(tools, LoadSkillName)
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c37e5bd: the analyst now exposes list_skills alongside load_skill whenever the project has skills, so the capped index fallback is actionable.

Comment on lines +111 to +113
if len(skills) > 0 {
tools = append(tools, LoadSkillName)
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c37e5bd: same change for the developer agent.

- Expose list_skills to the analyst and developer agents so the capped skill index fallback is actionable
- Gate the list_metrics_views skill enrichment and the MCP skills instructions on UseAI, matching the skill tools
- Give always-apply skills their own 32 KiB budget in list_metrics_views, separate from ai_instructions
- Render scoped always-apply skills with the metrics views they apply to (shared skillSection helper)
- Glob only the two skill roots for SKILL.md files instead of the whole repo
- Add Skill to the file-artifact initialization whitelist so reload attaches skill resources
- Drop the unused Skill.Path field and json tags
@nishantmonu51

Copy link
Copy Markdown
Collaborator Author

Addressed the suppressed findings from the latest Copilot review in c37e5bd: list_skills is exposed to both agents, the list_metrics_views skill enrichment and the MCP skills instructions are gated on UseAI, always-apply skills have their own 32 KiB budget separate from ai_instructions, scoped always-apply skills state which metrics views they apply to, the parser only globs the two skill roots, and Skill was added to the file-artifact initialization whitelist.

…rt files

- develop_file builds its own prompt without the parent conversation, so it now receives the developer skills (always-apply bodies, index, and the list_skills/load_skill tools) like the developer agent
- The parser ignores SQL/YAML files inside a skill directory (other than SKILL.md) on both full and incremental parses, so skill references and examples don't become project resources or parse errors
- Document that supporting files in a skill directory are ignored
The depth check excluded only nested support files, so skills/foo/example.sql and
.agents/skills/foo/config.yaml were still parsed as resources.
Comment thread admin/server/mcp.go
Comment on lines +55 to +56
ai.ListSkillsName,
ai.LoadSkillName,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but I'd worry that adding MCP tools for listing/loading skills might confuse the client since it may conflict with its local skill-loading abilities.

It appears MCP is working towards a native way for an MCP server to advertise additional skills, but it seems like the work is not stable yet: https://github.com/modelcontextprotocol/ext-skills/tree/main

Up to you whether you want to be cautious and wait until there is native MCP support before exposing this outside Rill's internal chat, or to just risk the conflicts/confusion.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping them exposed for now. Since 774c0d6 the tools are only advertised when the project defines skills, and MCP tools are namespaced per server, so a client with its own skill system only sees them when a project author opted in. Happy to revisit once ext-skills stabilises.

Comment on lines +134 to +135
- **`metrics_views`** scopes a skill to specific metrics views, so for example a marketing playbook is not offered during a finance analysis. It is a relevance filter, not access control. Referencing a metrics view that doesn't exist shows an error on the skill file, and the skill is not offered to the AI until the error is fixed.
- **`agents`** selects the agents the skill applies to: `analyst` for answering questions about your data, `developer` for editing the project's files. It defaults to `[analyst]`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be problematic to default to analyst? If you add normal coding agent skills to the project (e.g. the Rill development skills created by rill init or Clickhouse dev skill), then they would suddenly be exposed and loaded by the analyst agent (which could confuse it quite a lot).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and rill init makes it concrete: it writes the Rill development skills to .agents/skills/, which the analyst would have picked up. Changed the default to [developer] in b23270d; analysis skills opt in with agents: [analyst].

- **`agents`** selects the agents the skill applies to: `analyst` for answering questions about your data, `developer` for editing the project's files. It defaults to `[analyst]`.
- **`always_apply`** injects the skill's full contents into every conversation, like `ai_instructions`. Use it for short, broadly applicable guidance such as glossaries; keep always-apply skills small since they are included in every request. Always-apply skills share a 32 KiB budget per conversation; a skill that doesn't fit is offered for on-demand loading instead, and a warning is logged.

Other agent clients ignore Rill's extension fields, so a Rill skill remains a valid Agent Skill and vice versa. A skill directory may also hold supporting files (such as `references/` or `scripts/`) as the format allows; Rill ignores everything in a skill directory except `SKILL.md`, so a SQL or YAML example inside a skill is not parsed as a project resource.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so a SQL or YAML example inside a skill is not parsed as a project resource.

This sounds wrong. Any .sql or .yaml file inside a Rill project directory is parsed as a project resource, regardless of which subdirectory it is placed in.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a deliberate exception: the Agent Skills format allows scripts/ and references/ inside a skill directory, so pathIsSkillSupportFile in the parser ignores everything in a skill directory except SKILL.md, on both full and incremental parses (tests in parse_skill_test.go). Let me know if you would rather not special-case it.

Comment on lines +110 to +128
// Resolve the metrics views tied to the dashboard being explored, if any.
// This runs on every invocation because the metrics views scope the skills and the prompt context;
// only the pre-invoked tool calls below are limited to the first invocation.
var metricsViewNames []string
if args.Explore != "" {
_, metricsView, err := t.getValidExploreAndMetricsView(ctx, args.Explore)
if err != nil {
return nil, err
}
metricsViewNames = append(metricsViewNames, metricsView.Meta.Name.Name)
} else if args.Canvas != "" {
_, metricsViews, err := t.getValidCanvasAndMetricsViews(ctx, args.Canvas)
if err != nil {
return nil, err
}
for _, res := range metricsViews {
metricsViewNames = append(metricsViewNames, res.Meta.Name.Name)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyst may sometimes query other metrics views than are currently being looked at. So instead of customizing the user prompt at every turn, did you consider just pre-loading a list_skills tool call (which already returns info about always_apply or specific metrics views), and then using the analyst agent system prompt to generically instruct to it load skills relevant to the metrics views its currently exploring.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b23270d. The analyst pre-invokes list_skills on its first invocation, plus load_skill for each always-apply analyst skill, and analysis.md tells it to consider a skill's metrics_views for whichever views it queries. This removed skillPrompts, the prompt caps, and the metrics-view filtering.

Comment thread runtime/ai/develop_file.go Outdated
Comment on lines +214 to +221
{{ if .always_apply_skills }}
The user has defined the following skills that always apply to development work in this project. Follow their guidance:
{{ .always_apply_skills }}
{{ end }}
{{ if .skills_index }}
The user has defined the following development skills. Before doing work that a skill's description covers, you MUST call the "load_skill" tool to retrieve it and follow its instructions:
{{ .skills_index }}
{{ end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change in b23270d: develop_file pre-invokes list_skills and the always-apply developer skills alongside its other pre-invoked calls, and development.md carries the guidance.

Comment thread runtime/ai/developer_agent.go Outdated
Comment on lines +181 to +189
{{ if .always_apply_skills }}
The user has defined the following skills that always apply to development work in this project. Follow their guidance:
{{ .always_apply_skills }}
{{ end }}

{{ if .skills_index }}
The user has defined the following development skills. Before doing work that a skill's description covers, you MUST call the "load_skill" tool to retrieve it and follow its instructions:
{{ .skills_index }}
{{ end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change in b23270d: the developer agent pre-invokes list_skills and the always-apply developer skills alongside its other pre-invoked calls, and development.md carries the guidance.

Comment thread runtime/ai/mcp.go Outdated
Comment on lines +110 to +137
// Advertise the skills section of the instructions only when the project defines skills.
// It is resolved during the initialization handshake rather than when building the server,
// because the instructions are only returned on initialization while the server is rebuilt for every request.
srv.AddReceivingMiddleware(func(next mcp.MethodHandler) mcp.MethodHandler {
return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) {
res, err := next(ctx, method, req)
if method != "initialize" || err != nil {
return res, err
}
init, ok := res.(*mcp.InitializeResult)
if !ok {
return res, err
}
// Clients without UseAI are not offered the skill tools, so they are not told about skills either.
if !s.Claims().Can(runtime.UseAI) {
return init, nil
}
// On a load error, fail open with the skills section: it is harmless for projects without skills.
skills, skillsErr := s.Skills(ctx)
if skillsErr != nil {
s.logger.Warn("failed to load project skills", zap.Error(skillsErr))
}
if skillsErr != nil || len(skills) > 0 {
init.Instructions = MCPInstructions
}
return init, nil
}
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels kind of hacky. Did you consider using the same instructions, and just returning false from the skill-related tools' CheckAccess function when there are no skills? Then the tools will not be advertised when there are no skills without hacking the instructions. (That's what we do for other conditionally-available skills.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 774c0d6: list_skills and load_skill return false from CheckAccess when the project has no skills, and the instructions are static again.

Comment thread runtime/ai/metrics_view_list.go Outdated
Comment on lines +109 to +112
// Append always-apply skills so external clients receive them without extra round-trips.
// Skills are gated on UseAI like the skill tools, so a client that cannot use them does not receive their contents.
// Skill loading failures should degrade the response, not fail it.
if session.Claims().Can(runtime.UseAI) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a redundant check – if you can call MCP tools, then probably it's fine that you can read skills?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 774c0d6.

Comment thread runtime/ai/skills.go
Comment on lines +34 to +39
func (s *BaseSession) Skills(ctx context.Context) ([]*Skill, error) {
s.skillsMu.Lock()
defer s.skillsMu.Unlock()
if s.skillsLoaded {
return s.skills, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skillsMu does not respect ctx cancellations, which opens up potential bugs where request cancellations are not respected and cause resource leakage when/if loading the skills is blocked/stuck.

Would be safer to use our runtime/pkg/ctxsync.RWMutex utility, which is a mutex that supports context cancellations.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to ctxsync.RWMutex in 774c0d6.

…nstructions static

- list_skills and load_skill return false from CheckAccess when the project defines no skills, so they are not advertised; the MCP instructions no longer vary by project
- Drop the redundant UseAI check on the list_metrics_views skill enrichment
- Use ctxsync.RWMutex for the memoized skills so loading respects context cancellation
…jection; default skills to the developer agent

- Agents pre-invoke list_skills and load_skill for their always-apply skills instead of rendering skill bodies and an index into the user prompt; the system prompts describe how to use the tools (analyst: once per session, developer and develop_file: per invocation)
- Removes skillPrompts, the prompt size caps, and metrics-view filtering; the analyst decides relevance from the metrics_views field in the list_skills result
- Skills without agents now default to [developer], so coding skills (e.g. those rill init writes to .agents/skills) are not offered to the analyst; analysis skills set agents: [analyst]
- Update docs, the new-skill template, and tests
@nishantmonu51
nishantmonu51 merged commit cdefa6c into main Sep 15, 2026
35 checks passed
@nishantmonu51
nishantmonu51 deleted the nishant/ai-skills branch September 15, 2026 15:45
@nishantmonu51 nishantmonu51 mentioned this pull request Sep 21, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Size:L Large change: 500-1,999 lines Type:Feature New feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants